ldapccl,sql: validate ldap options provided in HBA config entry #132086
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @pritesh-lahoti)
pkg/ccl/ldapccl/ldap_test_util.go line 31 at r1 (raw file):
} var _ ILDAPUtil = &mockLDAPUtil{}
@rafiss I had to revert the commit 8290111#diff-778d7debed4f62a13dfa1790c041bd2e42101d67ec4fca08c20dde9abc9886d5R68 to make this a test file as I need the mockLDAPUtil in pkg/ccl/testccl/authccl to run the LDAP test and did not want to duplicate this. Also the duplication would need ldapConfig to be made a public struct and wanted to avoid that. Is there a cleaner way to do this or are you okay with the change?
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:
pkg/ccl/ldapccl/ldap_manager.go line 32 at r1 (raw file):
var ( enableUseCounter = telemetry.GetCounterOnce(enableCounterName) ldapSearchRe = regexp.MustCompile(`\(\s*\S+\s*=\s*\S+.*\)`)
nit: could we add details around how we came to this regex?
pkg/ccl/ldapccl/ldap_manager.go line 176 at r1 (raw file):
entryOptions[opt[0]] = true } // check for missing ldap options
nit: should this validation happen before the individual option parsing?
pkg/sql/pgwire/auth_methods.go line 88 at r1 (raw file):
// with a LDAP server. The remaining connection parameters are provided in hba // conf options. This auth method is re-registered in ldapccl module with // proper handler for CheckHBAEntry.
Do we still need to register it here in that case?
The func reads loadDefaultMethods and ldap is probably not one of those.
pkg/ccl/testccl/authccl/auth_test.go line 208 at r1 (raw file):
t.Fatal(err) } httpHBAUrl := httpScheme + s.HTTPAddr() + "/debug/hba_conf"
nit:
Suggestion:
httpHBAUrl := s.AdminURL().WithPath("/debug/hba_conf").String()pkg/ccl/testccl/authccl/auth_test.go line 444 at r1 (raw file):
return strconv.Itoa(resp.StatusCode), nil case "set_hba":
I don't quite see the set_hba command in the testdata file.
souravcrl
force-pushed
the
validate-ldap-options
branch
from
948be68
to
b56723b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/ldapccl/ldap_manager.go line 32 at r1 (raw file):
Previously, pritesh-lahoti wrote…
nit: could we add details around how we came to this regex?
done
pkg/ccl/ldapccl/ldap_manager.go line 176 at r1 (raw file):
Previously, pritesh-lahoti wrote…
nit: should this validation happen before the individual option parsing?
Umm I am populating the entryOptions map at the time of validating the option. This would need additional logic to populate the map before validating but eventually we will end up validating both number and correctness of options.
pkg/sql/pgwire/auth_methods.go line 88 at r1 (raw file):
Previously, pritesh-lahoti wrote…
Do we still need to register it here in that case?
The func readsloadDefaultMethodsand ldap is probably not one of those.
removed from here, we might risk registering an auth method for which doesn't have a check entry if auth method did not get re-registered.
pkg/ccl/testccl/authccl/auth_test.go line 208 at r1 (raw file):
Previously, pritesh-lahoti wrote…
nit:
done
pkg/ccl/testccl/authccl/auth_test.go line 444 at r1 (raw file):
Previously, pritesh-lahoti wrote…
I don't quite see the
set_hbacommand in the testdata file.
goland did not add the ldap test file. Have updated the commit now.
fixes CRDB-41624 Epic: CRDB-33829 Currently, we validate ldap configuration provided as HBA entry options at the time an auth request comes in for ldap. This prevents us from disallowing invalid/incomplete list of ldap options in HBA. This PR fixes the issue. Release note(security, ops): HBA config entry for LDAP will be evaluated with validations for proper ldap config parameter values and any invalid/incomplete options list will be disallowed to amend the HBA setting. We will validate all fields provided as ldap auth method options in HBA entry.
souravcrl
force-pushed
the
validate-ldap-options
branch
from
b56723b
to
4cd5def
Compare
There was a problem hiding this comment.
Nice exhaustive tests! Thank you, Sourav!
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status:
|
Thank you for the review! bors r=pritesh-lahoti |
132086: ldapccl,sql: validate ldap options provided in HBA config entry r=pritesh-lahoti a=souravcrl fixes CRDB-41624 Epic: CRDB-33829 Currently, we validate ldap configuration provided as HBA entry options at the time an auth request comes in for ldap. This prevents us from disallowing invalid/incomplete list of ldap options in HBA. This PR fixes the issue. Release note(security, ops): HBA config entry for LDAP will be evaluated with validations for proper ldap config parameter values and any invalid/incomplete options list will be disallowed to amend the HBA setting. We will validate all fields provided as ldap auth method options in HBA entry. Co-authored-by: souravcrl <[email protected]>
|
bors r- |
|
Canceled. |
|
bors r=pritesh-lahoti |
|
blathers backport 24.2 |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 4cd5def to blathers/backport-release-24.2-132086: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
fixes CRDB-41624
Epic: CRDB-33829
Currently, we validate ldap configuration provided as HBA entry options at the time an auth request comes in for ldap. This prevents us from disallowing invalid/incomplete list of ldap options in HBA. This PR fixes the issue.
Release note(security, ops): HBA config entry for LDAP will be evaluated with validations for proper ldap config parameter values and any invalid/incomplete options list will be disallowed to amend the HBA setting. We will validate all fields provided as ldap auth method options in HBA entry.